Skip to content

Conversation

@lbloder
Copy link
Collaborator

@lbloder lbloder commented Oct 20, 2025

📜 Description

Add @Configuration classes to initialize the profiler when running in OTEL Agent auto-init mode.

💡 Motivation and Context

Fixes #4855

💚 How did you test it?

📝 Checklist

  • I added GH Issue ID & Linear ID
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…options, add configuration class to load the profiler and converter after spring boot starts in agent mode
import org.springframework.context.annotation.Import;

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"})
Copy link
Collaborator Author

@lbloder lbloder Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it conditional on the async profiler class too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I see no reason to run this if profiler isn't there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What might become an issue is, if we ever have a second IContinuousProfiler implementation, we also have to update the condition

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 306.34 ms 341.79 ms 35.45 ms
Size 1.58 MiB 2.12 MiB 553.60 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ee747ae 374.71 ms 455.18 ms 80.47 ms
9fbb112 359.71 ms 421.85 ms 62.14 ms
23d6b12 354.10 ms 408.38 ms 54.28 ms
674d437 355.28 ms 504.18 ms 148.90 ms
604a261 380.65 ms 451.27 ms 70.62 ms
3d205d0 352.15 ms 432.53 ms 80.38 ms
ce0a49e 532.00 ms 609.96 ms 77.96 ms
d5a29b6 298.62 ms 391.78 ms 93.16 ms
ee747ae 358.21 ms 389.41 ms 31.20 ms
27d7cf8 306.76 ms 366.66 ms 59.90 ms

App size

Revision Plain With Sentry Diff
ee747ae 1.58 MiB 2.10 MiB 530.95 KiB
9fbb112 1.58 MiB 2.11 MiB 539.18 KiB
23d6b12 1.58 MiB 2.10 MiB 532.31 KiB
674d437 1.58 MiB 2.10 MiB 530.94 KiB
604a261 1.58 MiB 2.10 MiB 533.42 KiB
3d205d0 1.58 MiB 2.10 MiB 532.97 KiB
ce0a49e 1.58 MiB 2.10 MiB 532.94 KiB
d5a29b6 1.58 MiB 2.12 MiB 549.37 KiB
ee747ae 1.58 MiB 2.10 MiB 530.95 KiB
27d7cf8 1.58 MiB 2.12 MiB 549.42 KiB

Previous results on branch: feat/profiling-w-spring-otel-agent

Startup times

Revision Plain With Sentry Diff
b59fc8e 399.24 ms 444.62 ms 45.38 ms
c4729d6 315.52 ms 362.84 ms 47.32 ms
c773a3a 406.73 ms 491.63 ms 84.90 ms
3d7551b 388.73 ms 462.98 ms 74.24 ms
db75a56 331.37 ms 402.82 ms 71.45 ms
d76029f 339.65 ms 357.82 ms 18.17 ms
913e482 313.91 ms 378.00 ms 64.09 ms
30b9948 314.20 ms 354.71 ms 40.51 ms
3832729 297.73 ms 353.21 ms 55.48 ms
2b6b804 312.35 ms 370.80 ms 58.45 ms

App size

Revision Plain With Sentry Diff
b59fc8e 1.58 MiB 2.11 MiB 539.90 KiB
c4729d6 1.58 MiB 2.12 MiB 548.31 KiB
c773a3a 1.58 MiB 2.12 MiB 549.51 KiB
3d7551b 1.58 MiB 2.12 MiB 552.68 KiB
db75a56 1.58 MiB 2.12 MiB 551.94 KiB
d76029f 1.58 MiB 2.12 MiB 549.52 KiB
913e482 1.58 MiB 2.12 MiB 552.67 KiB
30b9948 1.58 MiB 2.12 MiB 551.90 KiB
3832729 1.58 MiB 2.12 MiB 551.94 KiB
2b6b804 1.58 MiB 2.12 MiB 551.86 KiB


@Bean
@ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConfiguration")
public IContinuousProfiler sentryOpenTelemetryProfilerConfiguration() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract init into util to streamline initialization code


@Bean
@ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConverterConfiguration")
public IProfileConverter sentryOpenTelemetryProfilerConverterConfiguration() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract init into util to streamline initialization code

@lbloder
Copy link
Collaborator Author

lbloder commented Nov 3, 2025

@sentry review

@lbloder
Copy link
Collaborator Author

lbloder commented Nov 3, 2025

@cursor review

@lbloder lbloder mentioned this pull request Nov 3, 2025
16 tasks
cursor[bot]

This comment was marked as outdated.

@lbloder lbloder marked this pull request as ready for review November 3, 2025 13:29
cursor[bot]

This comment was marked as outdated.

}

@Test
fun `initialize converter returns no-op converter if converter already initialized`() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m should it simply return the existing one instead of noop that potentially causes profiling to break?


@Bean
@ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConfiguration")
public IContinuousProfiler sentryOpenTelemetryProfilerConfiguration() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an expected order to these bean evaluations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about that again, and came to the conclusion that no, the order does not matter.

  1. It doesn't matter if the Profiler or the Converter are created first as they have no direct dependency to each other.
  2. Initially I was worried, that this needs to run after the original SentryAutoConfiguration, but based on the following scenarios it should not matter:
    a) No Agent -> Configuration does not run at all, as we depend on the AgentMarker
    b) Agent with AutoInit -> Sentry init happened through Agent and we set the profiler on the options. SentryAutoConfiguration shouldn't run at all as the dsn config should be done by OTEL. (caveat misconfiguration by also setting the dsn in application.properties)
    c1) Agent without AutoInit and AutoConfiguration runs before SentryProfilerConfiguration -> Sentry is enabled and profiler is already set if profiling is enabled, thus nothing happens.
    c2) Agent without AutoInit and SentryAutoConfiguration runs after SentryProfilerConfiguration -> Sentry is not yet enabled, thus SentryProfilerConfiguration does nothing and SentryAutoConfiguration configures profiler afterwards.

WDYT?

contextRunner
.withPropertyValues(
"sentry.dsn=http://key@localhost/proj",
"sentry.traces-sample-rate=1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also have "sentry.profile-session-sample-rate=1.0",?

"debug=true",
)
.run {
assertThat(it).hasSingleBean(IContinuousProfiler::class.java)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this currently simply asserting it has noop instances?

import org.springframework.context.annotation.Import;

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I see no reason to run this if profiler isn't there.

return previousOptions.getInitPriority().ordinal() <= newOptions.getInitPriority().ordinal();
}

public static IContinuousProfiler initializeProfiler(@NotNull SentryOptions options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m should we instead check if options already have a profiler / converter set (that's not noop) and return that?
IMO that makes it easier to reason about SDK init.
Looking at the tests it seems we're potentially replacing a working profiler / converter with noop (if misusing this util).

lbloder and others added 2 commits November 4, 2025 12:22
…Configuration.java

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return InitUtil.initializeProfiler(options);
} else {
return profiler;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: NoOp Profiler Overwrites Custom Configuration

When Sentry.isEnabled() returns false, the method creates and returns a new NoOpContinuousProfiler instance instead of returning the profiler already stored in options. This can inadvertently overwrite a custom profiler that was configured through OptionsConfiguration, as the bean will always return a fresh NoOp instance rather than preserving existing configuration.

Fix in Cursor Fix in Web

return InitUtil.initializeProfileConverter(options);
} else {
return converter;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Custom Configuration Unexpectedly Overwritten

When Sentry.isEnabled() returns false, the method creates and returns a new NoOpProfileConverter instance instead of returning the converter already stored in options. This can inadvertently overwrite a custom converter that was configured through OptionsConfiguration, as the bean will always return a fresh NoOp instance rather than preserving existing configuration.

Fix in Cursor Fix in Web

return InitUtil.initializeProfiler(options);
} else {
return profiler;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Sentry profiler: Custom configuration discarded.

When Sentry is disabled, the method returns a new NoOpContinuousProfiler instance instead of the existing profiler from options. This is inconsistent with sentry-spring/SentryProfilerConfiguration.java which returns options.getContinuousProfiler(). If options already has a configured profiler (e.g., set via custom configuration), returning a NoOp instance would discard it and potentially break profiling functionality. The unused local variable profiler suggests this was meant to be options.getContinuousProfiler().

Fix in Cursor Fix in Web

return InitUtil.initializeProfileConverter(options);
} else {
return converter;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Sentry Disabled: Configured Converter Ignored

When Sentry is disabled, the method returns a new NoOpProfileConverter instance instead of the existing converter from options. This is inconsistent with sentry-spring/SentryProfilerConfiguration.java which returns options.getProfilerConverter(). If options already has a configured converter, returning a NoOp instance would discard it and potentially break profile conversion. The unused local variable converter suggests this was meant to be options.getProfilerConverter().

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Profiler is not initialized when using spring-boot or spring with OTEL Agent

4 participants